Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the product of TaylorModels #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ulinares
Copy link
Contributor

This PR implements the product of TaylorModel's product as discussed in #80.

src/arithmetic.jl Outdated Show resolved Hide resolved
Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@lbenet
Copy link
Member

lbenet commented Jul 17, 2020

@mforets Do you have any comments on this PR?

@mforets
Copy link
Contributor

mforets commented Jul 17, 2020

I don't have further comments about the implementation, looks good to me 🎉

About the evaluation of the new method, it would be interesting to run the ARCH-NLN RE on TaylorModels#master and on this branch, and compare the accuacy measures such as the width of the final states. (This is now quite easy to setup since all results are stored in result/results.csv.)

@mforets
Copy link
Contributor

mforets commented Jul 17, 2020

I don't know for sure but maybe there is a difference only in models handling "small" numbers; in that sense, the Production-Destruction model is a good candidate.

@lbenet
Copy link
Member

lbenet commented Jul 17, 2020

About the evaluation of the new method, it would be interesting to run the ARCH-NLN RE on TaylorModels#master and on this branch, and compare the accuacy measures such as the width of the final states. (This is now quite easy to setup since all results are stored in result/results.csv.)

It seems a good suggestion to me; can you check that, @UzielLinares ?

@ulinares
Copy link
Contributor Author

I've checked out the suggested for the ProductionDestrucion model and found the following

Model Time Width Branch
PRDE20 I 3.0750348345000003 3.338181794366981e-20 master
PRDE20 P 8.14854831 5.386499535725456e-21 master
PRDE20 IP 5.807060152 1.0659289899163344e-20 master
PRDE20 I 3.6199666665000003 3.338181794366981e-20 current
PRDE20 P 9.537335849 5.386499535725456e-21 current
PRDE20 IP 6.833716878000001 1.0659289899163344e-20 current

This new implementation of the product takes 1.17x compared to the implementation on master.
Additionally, I've tested the discussed on issue #80 and found that the improvement in the bound is very small (of order 10^-14).

@lbenet
Copy link
Member

lbenet commented Jul 24, 2020

Though ~20% slower run times is not too bad, the actual improvement of the remainder using the proposed changes is not actually seen. My guess is that the changes may be seen for larger boxes.

I propose to leave this PR open, and try to speed up the functions involved, probably in IntervalArithmetics.jl.

@mforets @dpsanders: Do you have any comments or suggestions?

@mforets
Copy link
Contributor

mforets commented Jul 24, 2020

I've checked out the suggested for the ProductionDestrucion model and found the following

thanks for checking !

I propose to leave this PR open

sounds good to me. i'd also like to play a bit with the new method. we could benchmark remainder_product(a, b, aux, Δnegl) in isolation.

@lbenet
Copy link
Member

lbenet commented Jul 24, 2020

Another option is to run the benchmark suite in master and this branch, and check how different the results are with respect to time and also the final remainders. But this may be time consuming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants